Skip to content

fix: enable clippy as_conversions lint across workspace #5577

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
arianbeh wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix: enable clippy as_conversions lint across workspace #5577

arianbeh wants to merge 6 commits into from
+111 −62

Conversation

@arianbeh
Copy link

@arianbeh arianbeh commented Dec 10, 2025

Changes

  • Enabled the clippy::as_conversions lint at the workspace level by updating
    Cargo.toml.
  • Resolved all resulting as_conversions violations across the workspace by:
    • Replacing unsafe pointer casts with std::ptr::from_ref / std::ptr::from_mut
      where appropriate (e.g. in cpu-template-helper).
    • Replacing silent integer casts with explicit conversions such as
      i64::from(), u64::try_from(), and usize::try_from().
    • Using localized #[allow(clippy::as_conversions)] in low-level or const
      contexts where as is intentional and required (e.g., enum-to-byte
      encoding in ACPI AML, seccompiler const fns, and libc constant conversions).
    • Cleaning up comparisons and length/size conversions to avoid silent
      truncation.
  • Updated the necessary code paths in:
    • cpu-template-helper
    • utils::time
    • acpi-tables (AML, DSDT, MADT, XSDT)
    • seccompiler (bindings, types, lib)
    • rebase-snap
    • jailer (resource_limits)
    • snapshot-editor
    • firecracker (main and examples)
    • vmm (library, benches, and tests)

Reason

This PR implements the final step of #3161, consolidating several
cast-related Clippy lints into as_conversions and fixing all violations.

Enabling as_conversions ensures:

  • Integer and pointer casts are either:
    • Performed via explicit, safe conversion APIs, or
    • Clearly scoped behind #[allow] where they are intentional and unavoidable.
  • Silent truncation, wraparound, and lossy conversions are avoided.
  • The workspace stays clean under RUSTFLAGS="-Dwarnings".
  • Future code introducing unsafe as casts will be flagged consistently.

All changes preserve existing behavior while making the code safer and more
explicit about type boundaries.

Verification

Locally ran:

  • RUSTFLAGS="-Dwarnings" cargo clippy --all --all-targets
  • cargo fmt

All packages build clean with no Clippy warnings.

I was not able to run tools/devtool checkbuild or tools/devtool checkstyle
because /dev/kvm is not available in my environment.

Closes #3161.

@JamesC1305 JamesC1305 self-assigned this Dec 17, 2025
@JamesC1305
Copy link
Contributor

JamesC1305 commented Dec 19, 2025 •
edited
Loading

Hi @arianbeh,

Thank you for your contribution, this definitely looks like a good QoL change for us.

The only thing I'd suggest as an improvement is to use repr(u8) on enums you're defining From for. We can then use#[allow(clippy::as_conversions)] to allow a single as cast inside the from function. This doesn't prevent us using as entirely, but it makes our use case explicit, which should prevent any unwanted consequences whilst preventing unnecessary code bloat.

I'll leave a review on your PR and point it our :)

JamesC1305
JamesC1305 requested changes Dec 19, 2025
View reviewed changes
@JamesC1305
Copy link
Contributor

JamesC1305 commented Jan 7, 2026

Hi @arianbeh,

It looks like your changes are not compiling (https://buildkite.com/firecracker/firecracker-pr/builds/15430#019b98cb-47c2-4d53-8b05-b4cefbbac739/L698). Could you please fix this and squash it into your original commit?

@JamesC1305 JamesC1305 added the Status: Awaiting author Indicates that an issue or pull request requires author action label Jan 9, 2026
@JamesC1305
Copy link
Contributor

JamesC1305 commented Jan 15, 2026

Hi @arianbeh,

I notice you haven't engaged with this PR lately. Are you still making changes, or are you waiting for me to run tests?

@arianbeh
Copy link
Author

arianbeh commented Jan 15, 2026

Hi @arianbeh,

I notice you haven't engaged with this PR lately. Are you still making changes, or are you waiting for me to run tests?

Hey, I did fix the compile issue and I belive I pushed it. I also accpeted your earlier comments believing it would add those to the code.

@codecov
Copy link

codecov bot commented Jan 16, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 69.04762% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.23%. Comparing base (d61e343) to head (d36ea75).
⚠️ Report is 34 commits behind head on main.

Files with missing lines Patch % Lines
src/seccompiler/src/types.rs 0.00% 8 Missing ⚠️
src/acpi-tables/src/aml.rs 88.88% 3 Missing ⚠️
src/seccompiler/src/bindings.rs 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #5577   +/-   ##
=======================================
  Coverage   83.23%   83.23%           
=======================================
  Files         277      277           
  Lines       29262    29284   +22     
=======================================
+ Hits        24356    24376   +20     
- Misses       4906     4908    +2
Flag Coverage Δ
5.10-m5n.metal 83.58% <69.04%> (+<0.01%) ⬆️
5.10-m6a.metal 82.91% <69.04%> (+<0.01%) ⬆️
5.10-m6g.metal 80.19% <59.52%> (+<0.01%) ⬆️
5.10-m6i.metal 83.57% <69.04%> (+<0.01%) ⬆️
5.10-m7a.metal-48xl 82.90% <69.04%> (+<0.01%) ⬆️
5.10-m7g.metal 80.19% <59.52%> (+<0.01%) ⬆️
5.10-m7i.metal-24xl 83.55% <69.04%> (-0.01%) ⬇️
5.10-m7i.metal-48xl 83.55% <69.04%> (-0.01%) ⬇️
5.10-m8g.metal-24xl 80.19% <59.52%> (+<0.01%) ⬆️
5.10-m8g.metal-48xl 80.19% <59.52%> (+<0.01%) ⬆️
6.1-m5n.metal 83.60% <69.04%> (+<0.01%) ⬆️
6.1-m6a.metal 82.94% <69.04%> (+<0.01%) ⬆️
6.1-m6g.metal 80.19% <59.52%> (+<0.01%) ⬆️
6.1-m6i.metal 83.60% <69.04%> (+<0.01%) ⬆️
6.1-m7a.metal-48xl 82.93% <69.04%> (+<0.01%) ⬆️
6.1-m7g.metal 80.19% <59.52%> (+<0.01%) ⬆️
6.1-m7i.metal-24xl 83.61% <69.04%> (+<0.01%) ⬆️
6.1-m7i.metal-48xl 83.62% <69.04%> (+<0.01%) ⬆️
6.1-m8g.metal-24xl 80.19% <59.52%> (+<0.01%) ⬆️
6.1-m8g.metal-48xl 80.19% <59.52%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JamesC1305
Copy link
Contributor

JamesC1305 commented Jan 16, 2026

Hi @arianbeh,

It looks like style checks are failing due to rust formatting and commit messages. Rust formatting can be fixed using devtool fmt, and the best way to fix the commits would be to merge them into your original fix commit and force push. You can then run devtool checkstyle to verify that the tests pass. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JamesC1305 JamesC1305 JamesC1305 requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@JamesC1305 JamesC1305

Labels

Status: Awaiting author Indicates that an issue or pull request requires author action

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use clippy::as_conversions

2 participants

@arianbeh @JamesC1305